-
Notifications
You must be signed in to change notification settings - Fork 10
Optional deps #385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optional deps #385
Conversation
I've made all the format-specifc deps including cyvcf2 optional. |
Nice - so the idea is that if you just need to convert plink, you don't have to install cyvcf2? Infrastructure LGTM. I have some vague recollection of there being a recent PEP on handling this sort of thing, but haven't managed to track it down. @tomwhite, do you know what current best practices on handling optional deps is? |
Exactly - Maybe you are thinking of https://peps.python.org/pep-0631/ ?? |
Possibly this one? https://peps.python.org/pep-0771/ Optional packages can cause problems with conda (as in conda doesn't have a way to install them), but I'm not sure what the best practices there are. |
As far as I'm aware there in conda-land the "best" option is to have several packages e.g. |
I think you just have to do one big build in conda - I guess the good thing there is that you find out whether that's actually possible as you're building the package. Let's just build for pip for now, and worry about conda later. |
Aha, yes, thanks Tom - it was pep-771. So, we'd default to installing the VCF version if someone requests |
I'm not sure it is until 771 is adopted - I think we fall into https://peps.python.org/pep-0771/#packages-supporting-multiple-backends-or-frontends, where "installing the package without any extras results in a broken installation". So for now we just have to document the installation process clearly. |
3dd0fa4
to
a52a052
Compare
c3355e3
to
615fba3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -21,12 +21,7 @@ dependencies = [ | |||
"tabulate", | |||
"tqdm", | |||
"humanfriendly", | |||
# cyvcf2 also pulls in coloredlogs and click", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the comments here with the rationale for these dependencies. The only reason we're using coloredlogs is because it comes anyway with cyvcf2
I've added the new CI job to the required merge tests. |
No description provided.